Handle URL params for AptRepoFiles #3454
Conversation
There was a problem hiding this comment.
Thanks for the updates, although I'm a bit puzzled why the existing PR was closed (especially since the source branches of the old PR and this one are in ATIX fork...); anyway, this applies what I said back in the old PR: I'm switching it to draft, until the changes in katello are actually merged.
| @@ -461,19 +461,28 @@ def sections(self): | |||
|
|
|||
| def fix_content(self, content): | |||
| # Luckily apt ignores all Fields it does not recognize | |||
| baseurl = content["baseurl"] | |||
| parsed_url = urlparse(unquote(content["baseurl"])) | |||
There was a problem hiding this comment.
is unquote() still needed? see the discussion on this in #3223
There was a problem hiding this comment.
At least the Candlepin version available for Katello 4.13 (the latest supported release), currently Candlepin 4.4.14 is still affected. I have spent some time looking, but was not able to find any new information when or if Candlepin intends to go back to the old behavior. Given nothing has happened over several months, I can only assume it is not a major priority. The good news is that using unquote will work equally well for quoted and unquoted URLs. As a result I lean towards leaving it in, at least until there is any new information from Candlepin.
There was a problem hiding this comment.
It looks like that while some encoding issues in Candlepin were fixed, this does not appear to have changed that the baseurl used as part of these changes has its path part encoded. Not sure if this is intentional or another bug on Candlepin's part. The current state of these changes will work either way.
src/subscription_manager/repofile.py
Outdated
| @@ -461,19 +461,28 @@ def sections(self): | |||
|
|
|||
| def fix_content(self, content): | |||
| # Luckily apt ignores all Fields it does not recognize | |||
| baseurl = content["baseurl"] | |||
| parsed_url = urlparse(unquote(content["baseurl"])) | |||
| baseurl = parsed_url._replace(query="").geturl() | |||
| url_res = re.match(r"^https?://(?P<location>.*)$", baseurl) | |||
There was a problem hiding this comment.
TBH: if now baseurl is parsed as URL, then it'd be a good idea to replace this regexp to extract hostname/path/etc with the same urllib.parse APIs -- for example replacing the scheme to get the katello URL
no need to do it in the existing comment; either as additional commit on top of this, or as future PR/improvement
There was a problem hiding this comment.
After some trial and error I did add this change to the existing commit. Please let me know if I should separate it out again.
The last time I adopted a previously opened PR, without opening a new PR, this created small frictions in communication (because it was less clear who was talking to whom). There were small annoyances like lack of ability to "resolve threads". So we decided to try re-opening this time around. Probably does not make a major difference. 😉 |
quba42
force-pushed
the
handle_apt_repo_url_params
branch
3 times, most recently
from
6cf5ca0 to
bb752ff
Compare
This change allows 'AptRepoFiles' to handle URL parameters for "Components" and "Suites" giving it more flexibility rather than always using a fixed default value.
quba42
force-pushed
the
handle_apt_repo_url_params
branch
from
bb752ff to
5d83cd3
Compare
|
@ptoscano I have tried to answer your questions and implement the suggestions. Since the accompanying Katello change is now merged, I have undrafted the PR. |
This change allows 'AptRepoFiles' to handle URL parameters for "Components" and "Suites" giving it more flexibility rather than always using a fixed default value. If the URL supplied by Candlepin does not have any such URL parameters, then everything defaults back to the way it was before.
This change is needed to enable the following Katello feature: Katello/katello#11058
That being said, this change is fully backwards compatible and defines an interface that the Katello side PR will need to respect. The Katello PR is dependent on this PR, not the other way around.
This PR replaces the previous draft PR here: #3223
My apologies for the switch, the idea is to have both this and the Katello PR in the same hand, so I can better pursue the completion of both related PRs.